fix(Button): apply disabled styles while loading - #8412
anishtsudo wants to merge 3 commits into
Conversation
Apply existing aria-disabled visuals to loading buttons so a primary submit can look non-interactive without native disabled.
🦋 Changeset detectedLatest commit: 285b6a6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Loading link-style buttons still show an interactive underline on hover.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates loading buttons to use disabled styling while remaining focusable.
Changes:
- Applies disabled CSS to loading button variants.
- Adds primary-loading unit and visual coverage.
- Adds an
@primer/reactpatch changeset.
| File | Description |
|---|---|
ButtonBase.module.css |
Applies disabled styles to aria-disabled loading buttons. |
Button.features.stories.tsx |
Adds a loading-primary story. |
Button.docs.json |
Registers the new story. |
Button.test.tsx |
Tests loading styling and interaction behavior. |
Button.test.ts |
Adds visual regression coverage. |
loading-button-disabled-styles.md |
Documents the patch release. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| &:disabled, | ||
| &[aria-disabled='true']:not([data-loading='true']) { | ||
| &[aria-disabled='true'] { |
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
Thanks for working on this and adding tests! have a couple change requests
| export const LoadingPrimary = () => ( | ||
| <Button variant="primary" loading> | ||
| Submit | ||
| </Button> | ||
| ) | ||
|
|
There was a problem hiding this comment.
we already have a loading story https://primer.style/react/storybook/?path=/story/components-button-features--loading, can we expand that one to show the different variants (like https://primer.style/react/storybook/?path=/story/components-button-features--disabled) instead of creating a new one? 🙏🏽
There was a problem hiding this comment.
Updated the existing Loading story to show default, primary, danger, and invisible, same layout as Disabled. Removed the separate Loading Primary story.
There was a problem hiding this comment.
what purpose are these screenshots serving? can we remove them?
There was a problem hiding this comment.
Removed them. They were only for the PR description and were not used by tests.
Show every loading variant in the existing story, remove the review screenshots, and keep loading links from underlining on hover.
|
Addressed the review comments. The Loading story now shows the variants, and the screenshots are removed. Ready for another look. |
Closes #7879
Loading primary buttons already set
aria-disabledand droponClick, but CSS skipped disabled visuals wheneverdata-loadingwas true. Nativedisabledwas the only way to get the muted primary look, which is inaccessible for form submit buttons.This applies the existing
aria-disabledstyles to loading buttons.<Button variant="primary" loading>now looks like a disabled primary button, stays focusable, and does not use nativedisabled.Changelog
New
Changed
Button,IconButton, andLinkButtonnow use disabled visuals while remaining keyboard-focusableRemoved
Rollout strategy
This is a visual bug fix with no API change.
Testing & Reviewing
Loading,Disabled, andPrimaryin StorybookdisabledattributeonClickinactiveprimary styling is unchanged